🐛 Handle non-existing user IDs in read_user_by_id
#1396
Conversation
alejsdev
changed the title
read_user_by_id.read_user_by_id.
alejsdev
changed the title
read_user_by_id.read_user_by_id
|
Hello. I'd like to know why this pull request has not been approved. It is valid. |
|
Yeah pretty minor upgrade but it makes sens to merge IMO. |
There was a problem hiding this comment.
@saltie2193, thanks for working on this!
Please, take a look at my change suggestions.
Basically, I think we should leave existing tests untouched (only rename test_get_existing_user -> test_get_existing_user_as_superuser) and add 2 test for non-existing user (superuser - 404, regular user - 403)
| @pytest.mark.parametrize( | ||
| "is_superuser", (True, False), ids=lambda x: "superuser" if x else "normal user" | ||
| ) |
There was a problem hiding this comment.
Case with is_superuser=True is already covered by test_get_existing_user_as_superuser.
No need to parameterize this test
There was a problem hiding this comment.
It should not be the same as in test_get_existing_user_as_superuser.
test_get_existing_user_as_superuser tests whether a user authenticated as superuser is able to access another user. (User A tries to access data of user B)
test_get_exiting_user_current_user however tests whether a user is allowed to access his own data, independent of them being a superuser or not. (User A tries to access data of user A)
That's at least what they are intended to test.
There was a problem hiding this comment.
So, this is to ensure that superuser can access their own user information, not only other user's information, right?
I still think this is redundant. I believe the chance somebody writes the algorithm this way is extremely low..
There was a problem hiding this comment.
Yes. It's to ensure superuser privileges don't influence it.
Sure might be excessive but it's just an additional simple case of the test, ensuring the correct function of the backend, and not even expensive.
There was a problem hiding this comment.
I think I can come up with 10 more additional tests. And all of them will be hypothetically possible.
I think we should consider:
- what is the probability that somebody writes this function this way (superuser can access random user, but can't access their own info. At the same time normal user can access their own info)?
- every change is additional load for people who will review it and read this code later
There was a problem hiding this comment.
I mean especially when talking about authentication and authorization it do think it's not unlikely to not think about something like this, that seems really easy ans stupid. Especially when looking at differences between superusers and non superuser. Often the most simple parts are overlooked because people thought it's obvious how it's supposed to work and nobody would ever make a mistake like that.
This just uses as simple test case in a preventive way ensuring the backend is working as intended and at the same time explicitly documents the intended behavior.
But I do understand and have to respect it if this project does not want to use test cases in this way.
So we only test if the current user (authenticated as non-superuser) is able to access his data and I drop the changes to test_get_existing_user_current_user?
There was a problem hiding this comment.
I think we should keep test_get_existing_user_current_user as it was before this PR
Fix an issue where `read_user_by_id` would fail to return if the requested user ID did not exist. * Return `404 - Not Found` when ID does not exist. * Request without sufficient permission will always result in `403 - Unauthorized`. * Add tests to test requesting non-existing user IDs as superuser and normal user.
saltie2193
force-pushed
the
backend-read-user-by-id-no-user
branch
from
a2dd74f to
e031948
Compare
|
@YuriiMotov please have a look at the adjustments I made. I hope I could address all your concerns. Note, I rebased the original commits onto the master branch so |
There was a problem hiding this comment.
LGTM
I added a few suggestions that can reduce the diff a bit. Feel free to ignore
@saltie2193, thank you!
Co-authored-by: Motov Yurii <[email protected]>
|
From my side this is ready to merge. @YuriiMotov, thank you for taking the time to review this pull request! |
|
This pull request has a merge conflict that needs to be resolved. |
github-actions
bot
added
conflicts
# Conflicts: # backend/tests/api/routes/test_users.py
saltie2193
force-pushed
the
backend-read-user-by-id-no-user
branch
from
796afd5 to
568f0da
Compare
Fix an issue where
read_user_by_idwould fail to return if the requested user ID did not exist.404 - Not Foundwhen ID does not exist.403 - Unauthorized.